[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: onedr0p
Once this PR has been reviewed and has the lgtm label, please assign cooktheryan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Hi @onedr0p. Thanks for your PR.

I'm waiting for a backube member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Codecov Report

Merging #812 (f74f1f5) into main (49fe3ef) will decrease coverage by 0.1%.
The diff coverage is 63.6%.

@@           Coverage Diff           @@
##            main    #812     +/-   ##
=======================================
- Coverage   66.9%   66.9%   -0.1%     
=======================================
  Files         55      55             
  Lines       7058    7091     +33     
=======================================
+ Hits        4726    4747     +21     
- Misses      2054    2062      +8     
- Partials     278     282      +4     

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
13.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@onedr0p thanks for this PR, however I think if we do this there are a few updates we'd need to do:

• If we add immediate to replicationsource I'd prefer to also add to replicationdestination to be consistent
• Does it make sense to add immediate to spec.trigger somewhere? And potentially even call this something like spec.trigger.scheduleImmediate? Since I think this will not apply (and should be ignored) by manual triggers.
• I think the actual implementation will need to be modified to set status/modify the scheduling algorithm to make sure we skip this initial sync if immediate is false. The way it's done in the PR prevents the job from starting - which would be the same effect as creating a replicationsource with paused=true. The problem with this is everything else will still happen immediately - i.e. snapshot/clone will be taken of the pvc immediately, and that pvc from snap/clone will be of the PVC at the time when the replicationsource is created. I think to do this as you intend, we'd need to modify the scheduling to not sync at all at the start which is a little more involved as we don't want to risk breaking the scheduling.

I'll discuss with the rest of the team to see if this does sound like a valid direction to go - I will try to take a look at the scheduling when I have a moment and see if it's something that could be done relatively easily.

Closing PR, will circle back if needed after the volume populator functionality is released. I still think this might be needed for other situations but there's problems with this PR as pointed out above.